-
Notifications
You must be signed in to change notification settings - Fork 9
v1.3.0: Support Profile Detection for the Auto Profiles #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Bumps [nanoid](https://github.com/ai/nanoid) from 3.3.7 to 3.3.8. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.3.7...3.3.8) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Adapt firefox * support firefore auto publish * support firefox, and optimized UX
* [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * update English i18n msg --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: SYM01 <33443792+SYM01@users.noreply.github.com>
* [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
* [WIP] support chrome auto deploy * finalize Chrome auto deploy * npm audit fix * npm audit fix
* [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
Summary of ChangesHello @SYM01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant enhancement by enabling automatic detection and display of the proxy profile currently in use for each browser tab, particularly when 'Auto Profiles' are active. This feature aims to provide users with immediate feedback on their proxy configuration, improving transparency and control. The implementation required extensive modifications to the extension's core architecture, including new browser API integrations, a dedicated UI component for displaying detection results, and robust backend logic to accurately identify the active profile. Additionally, the project's dependencies have been updated to ensure stability and leverage the latest improvements. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant new feature: automatic profile detection for 'Auto Switch' mode. This is a substantial change that touches many parts of the extension, from the background script and browser adapters to new UI components and services. The implementation includes a PAC script simulator to determine which profile is active for a given tab, which is a clever way to work around browser limitations. The code is generally well-structured and is accompanied by a good set of new unit tests for the complex logic.
My review has identified a couple of critical issues that need to be addressed, particularly one that will break proxy authentication on Firefox and another bug in the profile detection logic. I've also pointed out some medium-severity issues, such as debug console.log statements that should be removed and a large dependency update that would be better handled in a separate pull request.
Overall, this is a great addition to the extension's functionality. Once the critical issues are resolved, this will be a solid contribution.
| browser.webRequest.onAuthRequired.addListener( | ||
| callback, | ||
| callback as any, | ||
| { urls: ["<all_urls>"] }, | ||
| ["asyncBlocking"] | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of onWebRequestAuthRequired for Firefox is incorrect. The browser.webRequest.onAuthRequired listener in Firefox requires a callback that returns a Promise<BlockingResponse>. The current implementation casts the callback to any, but the actual callback provided from background.ts is designed for Chrome's API and uses an asyncCallback argument instead of returning a promise. This will cause proxy authentication to fail on Firefox.
You need to wrap the callback in a new function that returns a Promise.
browser.webRequest.onAuthRequired.addListener(
(details) =>
new Promise((resolve) => {
callback(details, (response) => resolve(response));
}),
{ urls: ["<all_urls>"] },
["asyncBlocking"]
);| const ret = await profile.findProfileForAutoProfileRule( | ||
| url, | ||
| rule, | ||
| profile | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findProfileForAutoProfileRule is a private method of ProfileConverter. You are attempting to call it on profile, which is a different instance of ProfileConverter representing a sub-profile. This is incorrect as the method is part of the current instance's context. It should be called on this.
| const ret = await profile.findProfileForAutoProfileRule( | |
| url, | |
| rule, | |
| profile | |
| ); | |
| const ret = await this.findProfileForAutoProfileRule( | |
| url, | |
| rule, | |
| profile | |
| ); |
| "dependencies": { | ||
| "@highlightjs/vue-plugin": "^2.1.0", | ||
| "@sentry/vue": "^8.51.0", | ||
| "@vueuse/core": "^12.4.0", | ||
| "acorn": "^8.14.0", | ||
| "@sentry/vue": "^10.29.0", | ||
| "@typescript/native-preview": "^7.0.0-dev.20251205.1", | ||
| "@vueuse/core": "^14.1.0", | ||
| "acorn": "^8.15.0", | ||
| "escodegen": "^2.1.0", | ||
| "espree": "^10.3.0", | ||
| "espree": "^11.0.0", | ||
| "io-ts": "^2.2.22", | ||
| "ipaddr.js": "^2.2.0", | ||
| "vue": "^3.5.13", | ||
| "vue-router": "^4.5.0" | ||
| "ipaddr.js": "^2.3.0", | ||
| "vue": "^3.5.25", | ||
| "vue-router": "^4.6.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@arco-design/web-vue": "^2.56.3", | ||
| "@sentry/vite-plugin": "^3.0.0", | ||
| "@types/chrome": "^0.0.266", | ||
| "@arco-design/web-vue": "^2.57.0", | ||
| "@sentry/vite-plugin": "^4.6.1", | ||
| "@types/chrome": "^0.1.32", | ||
| "@types/escodegen": "^0.0.10", | ||
| "@types/espree": "^10.1.0", | ||
| "@types/firefox-webext-browser": "^120.0.4", | ||
| "@types/node": "^22.10.6", | ||
| "@vitejs/plugin-vue": "^5.2.1", | ||
| "@vitest/coverage-v8": "^3.0.6", | ||
| "rollup-plugin-visualizer": "^5.14.0", | ||
| "sass-embedded": "^1.83.3", | ||
| "typescript": "^5.7.3", | ||
| "unplugin-auto-import": "^19.0.0", | ||
| "unplugin-vue-components": "^28.0.0", | ||
| "vite": "^6.1.0", | ||
| "vitest": "^3.0.6", | ||
| "vue-tsc": "^2.2.2" | ||
| "@types/firefox-webext-browser": "^143.0.0", | ||
| "@types/node": "^24.10.1", | ||
| "@vitejs/plugin-vue": "^6.0.2", | ||
| "@vitest/coverage-v8": "^4.0.15", | ||
| "rollup-plugin-visualizer": "^6.0.5", | ||
| "sass-embedded": "^1.93.3", | ||
| "typescript": "^5.9.3", | ||
| "unplugin-auto-import": "^20.3.0", | ||
| "unplugin-vue-components": "^30.0.0", | ||
| "vite": "^7.2.6", | ||
| "vitest": "^4.0.15", | ||
| "vue-tsc": "^3.1.6" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR includes a large number of dependency updates, with several major version bumps. It's generally better to handle dependency upgrades in a separate PR from feature work. This isolates any potential breaking changes or regressions from the dependency updates, making them easier to identify and fix.
| // this.stats.addFailedRequest(details); | ||
| // TODO: update indicator | ||
| const proxySetting = await getCurrentProxySetting(); | ||
| console.log("onResponseStarted", details); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // mocking | ||
| // tabStats.value = { | ||
| // failedRequests: [], | ||
| // currentProfile: { | ||
| // profile: { | ||
| // profileID: "profile1", | ||
| // profileName: "Profile 1", | ||
| // proxyType: "proxy", | ||
| // color: "#000000", | ||
| // proxyRules: { | ||
| // default: { | ||
| // host: "127.0.0.1", | ||
| // port: 8080, | ||
| // }, | ||
| // bypassList: [], | ||
| // }, | ||
| // }, | ||
| // isConfident: true, | ||
| // }, | ||
| // tabID: props.currentTab.id, | ||
| // }; | ||
| // console.log(tabStats.value); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return { | ||
| validator: async (value, cb) => { | ||
| validator: async (value: string, cb: (message?: string) => void) => { | ||
| console.log("test"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the extension supports auto detection of the profile being used for the current tab.
But due to the limitations of the modern browsers, this detection might not always accurate. The limitations are: